Repository navigation
🐛 dashboard: stabilize scroll across all repaint paths - #17458
Conversation
Content above the viewport changes height on SSE panel updates, lazy section loads, sparkline replays and pane polls, not only on render(), so the reader's section drifted up or down. Add a standalone scroll stabilizer: a ResizeObserver on the document and every [data-dashboard-section] re-applies a continuously maintained anchor (same shape as captureScrollAnchor) after layout and before paint, so the correction lands in the frame of the shift. Delta-based against a persistent node, it composes with native scroll anchoring and defers to hash/sidebar navigation settling. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/L
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of
|
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Style/maintainability review at head fecae86 (read static-src/js/scroll-stabilizer.js, refresh.js, agent-detail.js, progressive.js, and the existing scroll tests).
1. No pinning test, unlike every prior scroll fix in this tree — src/pkg/dashboard/static-src/js/scroll-stabilizer.js:1-110
The repo idiom for dashboard scroll behaviour is a jsFunc-snippet Go test that pins the mechanism (agents_refresh_scroll_jump_test.go:22-43 for the card anchor, sidebar_nav_scroll_settle_test.go for ocSettleScroll). This PR adds 110 lines of scroll-correction logic and no such pin, so a future edit can silently drop the _hashScrollPending/_ocScrollSettleCancel deferral or the _ssSelfScroll guard. Suggested src/pkg/dashboard/scroll_stabilizer_test.go:
+package dashboard
+
+import (
+ "strings"
+ "testing"
+)
+
+// TestScrollStabilizerDefersToNavigation pins the ResizeObserver-driven
+// stabilizer: it must never fight a settling hash/sidebar navigation, and
+// must not treat its own scrollBy as a reader scroll.
+func TestScrollStabilizerDefersToNavigation(t *testing.T) {
+ html := indexHTML(t)
+ suppressed := jsFunc(t, html, "_ssSuppressed")
+ for _, snippet := range []string{"_hashScrollPending) return true;", "_ocScrollSettleCancel) return true;"} {
+ if !strings.Contains(suppressed, snippet) {
+ t.Errorf("_ssSuppressed is missing %q", snippet)
+ }
+ }
+ correct := jsFunc(t, html, "_ssCorrect")
+ for _, snippet := range []string{"if (_ssSuppressed() || !_ssAnchor || window.scrollY <= 0)", "_ssSelfScroll = true;", "window.scrollBy(0, delta);"} {
+ if !strings.Contains(correct, snippet) {
+ t.Errorf("_ssCorrect is missing %q", snippet)
+ }
+ }
+ init := jsFunc(t, html, "initScrollStabilizer")
+ if !strings.Contains(init, "new ResizeObserver(_ssCorrect)") {
+ t.Error("initScrollStabilizer no longer drives correction from a ResizeObserver")
+ }
+}2. _ssDrift re-implements restoreScrollAnchor's lookup — scroll-stabilizer.js:45-57 vs refresh.js:441-457
The CSS.escape fallback, .agent-card[data-agent="…"] query and getClientRects().length checks are copied verbatim. The two will drift apart (e.g. if the card selector changes). Extract the delta into one helper in refresh.js and have both callers use it:
--- a/src/pkg/dashboard/static-src/js/refresh.js
+++ b/src/pkg/dashboard/static-src/js/refresh.js
- function restoreScrollAnchor(anchor) {
+ // Vertical distance the anchor has moved since capture, or null when the
+ // anchored node is gone and the caller must fall back to scrollY.
+ function scrollAnchorDrift(anchor) {
if (anchor.cardKey && anchor.el && anchor.el.isConnected) {
const safeKey = (window.CSS && CSS.escape) ? CSS.escape(anchor.cardKey) : String(anchor.cardKey).replace(/["\\]/g, '\\$&');
const card = anchor.el.querySelector(`.agent-card[data-agent="${safeKey}"]`);
- if (card && card.getClientRects().length) {
- const delta = card.getBoundingClientRect().top - anchor.cardTop;
- if (delta) window.scrollBy(0, delta);
- return;
- }
+ if (card && card.getClientRects().length) return card.getBoundingClientRect().top - anchor.cardTop;
}
if (anchor.el && anchor.el.isConnected && anchor.el.getClientRects().length) {
- const delta = anchor.el.getBoundingClientRect().top - anchor.top;
- if (delta) window.scrollBy(0, delta);
- return;
+ return anchor.el.getBoundingClientRect().top - anchor.top;
}
- window.scrollTo(0, anchor.scrollY);
+ return null;
+ }
+
+ function restoreScrollAnchor(anchor) {
+ const delta = scrollAnchorDrift(anchor);
+ if (delta === null) { window.scrollTo(0, anchor.scrollY); return; }
+ if (delta) window.scrollBy(0, delta);
}
--- a/src/pkg/dashboard/static-src/js/scroll-stabilizer.js
+++ b/src/pkg/dashboard/static-src/js/scroll-stabilizer.js
- // Returns the vertical delta the anchor has drifted by, or 0.
- function _ssDrift(anchor) {
- if (!anchor) return 0;
- if (anchor.cardKey && anchor.el && anchor.el.isConnected) {
- const safeKey = (window.CSS && CSS.escape) ? CSS.escape(anchor.cardKey) : String(anchor.cardKey).replace(/["\\]/g, '\\$&');
- const card = anchor.el.querySelector(`.agent-card[data-agent="${safeKey}"]`);
- if (card && card.getClientRects().length) return card.getBoundingClientRect().top - anchor.cardTop;
- }
- if (anchor.el && anchor.el.isConnected && anchor.el.getClientRects().length) {
- return anchor.el.getBoundingClientRect().top - anchor.top;
- }
- return 0;
- }
+ // Returns the vertical delta the anchor has drifted by, or 0 when the
+ // anchored node is gone (the next capture will pick a new one).
+ function _ssDrift(anchor) {
+ if (!anchor) return 0;
+ return scrollAnchorDrift(anchor) || 0;
+ }Both changes need make dashboard-gen to regenerate static/index.html. Everything else (typeof guards matching progressive.js:140, _ss prefix, ── ── header, DOMContentLoaded gate matching layout.js:325) follows existing conventions.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor. Review findings from Intent-alignment review (head The body says the stabilizer "defers to
Suggested fix — route the three through the existing settle loop so the stabilizer's guard sees them (shown for --- a/src/pkg/dashboard/static-src/js/agent-detail.js
+++ b/src/pkg/dashboard/static-src/js/agent-detail.js
@@ function scrollToAgentCard(name, behavior) {
const card = document.querySelector(selector);
if (!card) return false;
- card.scrollIntoView({ block: 'center', behavior: behavior || 'smooth' });
+ const targetY = () => card.getBoundingClientRect().top + window.scrollY - Math.max(0, (window.innerHeight - card.offsetHeight) / 2);
+ ocSettleScroll(targetY, () => card.scrollIntoView({ block: 'center', behavior: behavior || 'smooth' }), OC_NAV_SETTLE_MS, card);
card.classList.remove('anchor-highlight');Passing Not verified by running the dashboard; inferred from the frame ordering (scroll event → rAF → layout → ResizeObserver) and the fact that any programmatic scroll aborts a pending smooth scroll in Chromium/Gecko. If the author has observed these paths staying smooth under SSE load with this branch, I'd take that as evidence against this. Review findings from Correctness pass on Deferred recapture absorbs back-to-back shifts (medium) — Fix: re-capture synchronously inside the RO callback. Layout is already clean there and function _ssCorrect() {
- if (_ssSuppressed() || !_ssAnchor || window.scrollY <= 0) { _ssQueueCapture(); return; }
+ if (_ssSuppressed() || !_ssAnchor || window.scrollY <= 0) { _ssCapture(); return; }
const delta = _ssDrift(_ssAnchor);
if (Math.abs(delta) >= SCROLL_STABILIZER_MIN_DELTA_PX) {
const before = window.scrollY;
_ssSelfScroll = true;
window.scrollBy(0, delta);
// Clamped at an edge: no scroll event will follow, so clear the flag.
if (window.scrollY === before) _ssSelfScroll = false;
}
- // Content may have changed which section is topmost; refresh the
- // anchor so the next shift is measured from where the reader is now.
- _ssQueueCapture();
+ // Content may have changed which section is topmost; refresh the
+ // anchor now (layout is clean inside the RO callback) so a shift that
+ // lands before the next frame is still measured against this position.
+ _ssCapture();
}Keep the rAF-coalesced path for the Not verified by test: there is no JS harness for this behaviour — only Review findings from Read One behavioural interaction worth a decision before this lands: Un-wrapped smooth scrolls get aborted mid-flight (medium). Not a defect, but a convention gap: this repo pins its scroll JS with structural Go tests ( Body matches the diff (no claims beyond what Review findings from docs-currency (read at head fecae86) The new
Everything else checked out: the anchor shape ( |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Problem
While reading one section of the dashboard, other parts of the page grow and shrink (SSE panel updates, lazy section loads, sparkline replays, agent pane polls) and the content under the reader's eyes moves.
captureScrollAnchor/restoreScrollAnchor(#11032) only wraprender()and the progressive loader; every other repaint path bypasses them. Browser-native scroll anchoring does not cover it either, since innerHTML rerenders replace the anchor node.Fix
New
static-src/js/scroll-stabilizer.js:[data-dashboard-section], or first visible agent card bydata-agentkey once scrolled into the grid) — recaptured on user scroll (passive, rAF-coalesced) and after each correction;ResizeObserverondocumentElement,bodyand every section calls the corrector after layout, before paint, so the fix lands in the same frame as the shift and nothing visibly moves;MutationObserverpicks up lazily rendered sections (observe()is idempotent);_hashScrollPendingand an activeocSettleScrollso hash/sidebar navigation is never fought; viewportresizere-measures instead of correcting; no-op atscrollY <= 0.index.htmlregenerated withmake dashboard-gen.Verification
go test ./pkg/dashboard/...— webstatic/gen pass (TestIndexHTMLIsGeneratedgreen); the only failure inpkg/dashboardis the pre-existingTestKnowledgeDrainLegacyMemorymacOS/varsymlink issue, unrelated.